Skip to content

Fix inpainting robustness: correct NaN propagation and fill value#146

Open
simone-silvestri wants to merge 3 commits intomainfrom
ss/fix-inpainting-robustness
Open

Fix inpainting robustness: correct NaN propagation and fill value#146
simone-silvestri wants to merge 3 commits intomainfrom
ss/fix-inpainting-robustness

Conversation

@simone-silvestri
Copy link
Copy Markdown
Member

Summary

  • Fix _propagate_field! to use donors == 0 instead of value == 0 for detecting cells with no valid neighbors — the old check incorrectly marked cells as NaN when valid neighbors averaged to zero
  • Fix _fill_nans! to fill remaining NaN with the field mean instead of 0 — using 0 is catastrophic for fields like salinity (~34 psu) and can destabilize simulations
  • Update test to reflect the new fill behavior

MWE demonstrating the bugs

using Oceananigans
using NumericalEarth.DataWrangling: NearestNeighborInpainting, inpaint_mask!

grid = RectilinearGrid(CPU(); size=(10, 10, 1), x=(0, 1), y=(0, 1), z=(0, 1))

# --- Bug 1: value == 0 vs donors == 0 ---
# A masked cell surrounded by neighbors that average to zero
# (e.g. temperature data near 0°C with values -2, +2, -2, +2)
field = CenterField(grid)
mask = Field{Center, Center, Center}(grid, Bool)
interior(field) .= 0.0
interior(field)[4, 5, 1] = -2.0
interior(field)[6, 5, 1] =  2.0
interior(field)[5, 4, 1] = -2.0
interior(field)[5, 6, 1] =  2.0
interior(mask)[5, 5, 1] = true

inpaint_mask!(field, mask; inpainting=NearestNeighborInpainting(100))
@show interior(field)[5, 5, 1]
# Before fix: NaN → then 0 (via _fill_nans!). Wrong code path.
# After fix:  0.0 directly from averaging. Correct.

# --- Bug 2: _fill_nans! converts NaN → 0 instead of field mean ---
# A salinity-like field (~34 psu) with a large masked region
# and finite maxiter so the center can't be reached by propagation
field2 = CenterField(grid)
mask2 = Field{Center, Center, Center}(grid, Bool)
set!(field2, (x, y, z) -> 34.0)
for i in 3:7, j in 3:7
    interior(mask2)[i, j, 1] = true
end
inpaint_mask!(field2, mask2; inpainting=NearestNeighborInpainting(1))
@show interior(field2)[5, 5, 1]
# Before fix: 0.0 (from _fill_nans! multiplying by !isnan). Catastrophic for salinity.
# After fix:  34.0 (filled with field mean). Physically reasonable.

Test plan

  • Verified donors == 0 fix: zero-average neighbors correctly inpaint to 0.0
  • Verified _fill_nans! fix: unreachable cells fill with field mean (~34) instead of 0
  • Verified EN4 inpainted data: interior has zero NaN, zero zeros after fix
  • Existing test_inpainting_algorithm passes with updated expectations
  • Run half-degree OMIP simulation with fresh inpainted cache to confirm no NaN crash

🤖 Generated with Claude Code

simone-silvestri and others added 2 commits March 26, 2026 14:56
Two bugs in the nearest-neighbor inpainting algorithm:

1. `_propagate_field!` used `value == 0` instead of `donors == 0` to detect
   cells with no valid neighbors. When valid neighbors average to zero
   (e.g. temperature near 0°C), cells were incorrectly marked as NaN.

2. `_fill_nans!` converted remaining NaN cells to 0, which is catastrophic
   for fields like salinity (~34 psu). Now fills with the mean of valid
   interior data instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/DataWrangling/inpainting.jl 57.14% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant